F-1899 : fix potential heap buffer over-read by miyazakh · Pull Request #220 · wolfSSL/wolfCLU

miyazakh · 2026-04-08T07:24:55Z

Fix potential heap buffer over-read
Refactor wolfCLU_sign_data_dilithium
Add test coverage

Depend on : #219

Copilot

Pull request overview

This PR addresses robustness and safety in signing/verification workflows, primarily targeting Dilithium signing error handling to prevent potential heap misuse, and extends regression coverage via shell tests.

Changes:

Refactors wolfCLU_sign_data_dilithium to improve cleanup/error-path handling and avoid unsafe buffer usage patterns.
Updates ECC sign/verify to hash input data before calling ECDSA primitives.
Adds Dilithium negative-path regression tests to ensure failures don’t create output files.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File	Description
tests/genkey_sign_ver/genkey-sign-ver-test.sh	Adds Dilithium failure-mode tests (invalid output path, wrong key type, corrupted key) and cleans up new artifacts.
src/x509/clu_x509_sign.c	Adjusts `wolfSSL_BIO_get_fp` calls (casts) while loading key material for Chimera cert signing.
src/sign-verify/clu_verify.c	Changes ECC verification to hash input data prior to `wc_ecc_verify_hash`.
src/sign-verify/clu_sign.c	Changes ECC signing to hash input data prior to `wc_ecc_sign_hash`; refactors Dilithium signing for safer handling.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

dgarske

🐺 Skoll Code Review

Overall recommendation: REQUEST_CHANGES
Findings: 3 total — 2 posted, 1 skipped

Posted findings

[High] wc_dilithium_init failure not captured in ret — execution continues on error — src/sign-verify/clu_sign.c:648-650
[Medium] Error log for allocation failure prints stale ret value (always 0) — src/sign-verify/clu_sign.c:724-725

Skipped findings

[Info] Extra blank line where XMEMSET was removed

Review generated by Skoll via openclaw

add test coverage

migrate tests to python

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-17T04:37:04Z

    keyFile = XFOPEN(keyPath, "rb");
    if (keyFile == NULL) {
-        wolfCLU_LogError("Faild to open Private key FILE.");
+        wolfCLU_LogError("Failed to open Private key FILE.");


The error path when opening the Dilithium public key logs "Failed to open Private key FILE." even though this function is reading a public key (see the preceding comment "open and read public key"). Please correct the message to reference the public key to avoid confusing diagnostics.

Suggested change

wolfCLU_LogError("Failed to open Private key FILE.");

wolfCLU_LogError("Failed to open Public key FILE.");

Copilot · 2026-04-17T04:37:04Z

+            XFWRITE(outBuf, 1, outBufSz, outFile);
+            XFCLOSE(outFile);


Signature write success isn’t validated: if XFWRITE() fails (disk full, permission issues, etc.), ret remains 0 and the function will return success. Check the XFWRITE() result and convert failures into an error code (and ideally avoid leaving a partially-written output file).

Suggested change

XFWRITE(outBuf, 1, outBufSz, outFile);

XFCLOSE(outFile);

size_t written = XFWRITE(outBuf, 1, outBufSz, outFile);

if (written != outBufSz) {

wolfCLU_LogError("Failed to write signature to output file %s",

out);

ret = IO_FAILED_E;

}

if (XFCLOSE(outFile) != 0 && ret == 0) {

wolfCLU_LogError("Failed to close output file %s", out);

ret = IO_FAILED_E;

}

if (ret != 0) {

XREMOVE(out);

}

Copilot · 2026-04-17T04:37:05Z

+        with open(corrupt_key, "w") as f:
+            f.write("INVALID KEY DATA")


This test writes a file intended to be parsed as DER (-inform der), but opens it in text mode ("w"). On Windows this can introduce newline translation and make the test less predictable. Open the corrupted key file in binary mode ("wb") for consistency with other tests (e.g., RSA invalid key test).

Suggested change

with open(corrupt_key, "w") as f:

f.write("INVALID KEY DATA")

with open(corrupt_key, "wb") as f:

f.write(b"INVALID KEY DATA")

Copilot · 2026-04-17T04:37:05Z

+        if (XFSEEK(privKeyFile, 0, SEEK_SET) != 0 ||
+            (int)XFREAD(privBuf, 1, privFileSz, privKeyFile) != privFileSz) {
+            wolfCLU_LogError("Incorrect private key file size: %d", privFileSz);
+            ret = WOLFCLU_FATAL_ERROR;
+        }


This failure condition covers both seek/read errors (not just size mismatches), but the log message always reports an "Incorrect private key file size". Consider logging a more accurate read/seek failure message (or splitting the checks) to make debugging I/O errors easier.

Suggested change

if (XFSEEK(privKeyFile, 0, SEEK_SET) != 0 ||

(int)XFREAD(privBuf, 1, privFileSz, privKeyFile) != privFileSz) {

wolfCLU_LogError("Incorrect private key file size: %d", privFileSz);

ret = WOLFCLU_FATAL_ERROR;

}

if (XFSEEK(privKeyFile, 0, SEEK_SET) != 0) {

wolfCLU_LogError("Failed to seek to beginning of private key file.");

ret = WOLFCLU_FATAL_ERROR;

}

else if ((int)XFREAD(privBuf, 1, privFileSz, privKeyFile) != privFileSz) {

wolfCLU_LogError("Failed to read private key file: expected %d bytes.",

privFileSz);

ret = WOLFCLU_FATAL_ERROR;

}

Copilot AI review requested due to automatic review settings April 8, 2026 07:24

miyazakh self-assigned this Apr 8, 2026

Copilot started reviewing on behalf of miyazakh April 8, 2026 07:25 View session

Copilot AI reviewed Apr 8, 2026

View reviewed changes

Comment thread src/sign-verify/clu_sign.c Outdated

Comment thread src/sign-verify/clu_sign.c Outdated

Comment thread src/sign-verify/clu_verify.c

Comment thread src/sign-verify/clu_sign.c

miyazakh force-pushed the f-1899_heapbuffer_over-read branch from dc2f472 to 4328349 Compare April 8, 2026 23:25

miyazakh marked this pull request as ready for review April 8, 2026 23:41

Copilot AI review requested due to automatic review settings April 8, 2026 23:41

Copilot started reviewing on behalf of miyazakh April 8, 2026 23:41 View session

Copilot AI reviewed Apr 8, 2026

View reviewed changes

Comment thread src/sign-verify/clu_sign.c

Comment thread src/sign-verify/clu_sign.c

Comment thread src/sign-verify/clu_sign.c

Comment thread src/sign-verify/clu_sign.c

miyazakh assigned wolfSSL-Bot and unassigned miyazakh Apr 9, 2026

dgarske requested changes Apr 10, 2026

View reviewed changes

Comment thread src/sign-verify/clu_sign.c Outdated

Comment thread src/sign-verify/clu_sign.c

dgarske assigned miyazakh and unassigned wolfSSL-Bot Apr 10, 2026

miyazakh added 3 commits April 17, 2026 11:58

fix f-1899 potential heap buffer over-read

64fe29e

add test coverage

addressed Copilot review comments

84cfd0c

Addressed code review

7d03ee9

migrate tests to python

miyazakh force-pushed the f-1899_heapbuffer_over-read branch from 620f391 to 7d03ee9 Compare April 17, 2026 04:14

Addressed review comments

95c91af

Copilot AI review requested due to automatic review settings April 17, 2026 04:33

Copilot started reviewing on behalf of miyazakh April 17, 2026 04:33 View session

Copilot AI reviewed Apr 17, 2026

View reviewed changes

	wolfCLU_LogError("Failed to open Private key FILE.");
	wolfCLU_LogError("Failed to open Public key FILE.");

-            XFWRITE(outBuf, 1, outBufSz, outFile);
-            XFCLOSE(outFile);
+            size_t written = XFWRITE(outBuf, 1, outBufSz, outFile);
+            if (written != outBufSz) {
+                wolfCLU_LogError("Failed to write signature to output file %s",
+                                 out);
+                ret = IO_FAILED_E;
+            }
+            if (XFCLOSE(outFile) != 0 && ret == 0) {
+                wolfCLU_LogError("Failed to close output file %s", out);
+                ret = IO_FAILED_E;
+            }
+            if (ret != 0) {
+                XREMOVE(out);
+            }

		with open(corrupt_key, "w") as f:
		f.write("INVALID KEY DATA")

Conversation

miyazakh commented Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dgarske left a comment

Choose a reason for hiding this comment

🐺 Skoll Code Review

Posted findings

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

miyazakh commented Apr 8, 2026 •

edited

Loading